Skip to content

Conversation

linux019
Copy link

The check resp.ContentLength <= maxBodySlurpSize supersedes resp.ContentLength == -1 as ContentLength is signed int64
https://play.golang.org/p/Io9U59wxxkK

@google-cla
Copy link

google-cla bot commented Aug 21, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Aug 21, 2020
@codecov
Copy link

codecov bot commented Aug 21, 2020

Codecov Report

Merging #1615 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1615   +/-   ##
=======================================
  Coverage   68.04%   68.04%           
=======================================
  Files          97       97           
  Lines        8830     8830           
=======================================
  Hits         6008     6008           
  Misses       1908     1908           
  Partials      914      914           
Impacted Files Coverage Δ
github/github.go 89.93% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd957f2...9a6d050. Read the comment docs.

// won't reuse it anyway.
const maxBodySlurpSize = 2 << 10
if resp.ContentLength == -1 || resp.ContentLength <= maxBodySlurpSize {
if resp.ContentLength <= maxBodySlurpSize {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this instead?

Suggested change
if resp.ContentLength <= maxBodySlurpSize {
if resp.ContentLength != 0 && resp.ContentLength <= maxBodySlurpSize {

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And while you are making tweaks, how about wrapping lines 572-576 with a check like:

if resp != nil {
...
}

@itsksaurabh
Copy link
Contributor

@gmlewis @linux019
The value -1 indicates that the length is unknown.

 // ContentLength records the length of the associated content.
    // The value -1 indicates that the length is unknown.
    // Values >= 0 indicate that the given number of bytes may
    // be read from Body.
    //
    // For client requests, a value of 0 with a non-nil Body is
    // also treated as unknown.
    ContentLength int64

Please check it here. https://golang.org/pkg/net/http/

@gmlewis
Copy link
Collaborator

gmlewis commented Nov 12, 2020

Closing this PR in favor of #1645.

@gmlewis gmlewis closed this Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Indication that the PR author has signed a Google Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants